This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rules-and-rule-templates-yaml-integration/168568/304

@openhab/core-maintainers I'm not able to figure out why the JavaDoc check fails. I've built org.openhab.core.automation locally, and there are lots of warnings, but I haven't found anything that seems particularly related to this PR. I can't find a "report file" for JavaDoc to check either. Does anybody have any hints?

I see a number of errors in the build log similar to this, but I don't know what this PR does to affect that, or why it expects site to be populated in the first place:

Error:  Error fetching link: /home/runner/work/openhab-core/openhab-core/bom/compile/target/site/apidocs. Ignored it.
Error:  Error fetching link: /home/runner/work/openhab-core/openhab-core/bom/test/target/site/apidocs. Ignored it.

I think I found and fixed the JavaDoc problems, they weren't exactly easy to find. For future reference, I used this command to find them:

mvn javadoc:javadoc -X -Dshow=protected -pl :org.openhab.core.automation

@lolodomo Here it is: Have at it 😉

I must update this PR to accommodate #5462. Hopefully, it won't require large changes.

The changes to adjust to #5462 were simple except that now I can't get the formatter to behave properly regarding indentation. Having two optional sections have made it more complicated to handle indentation, and the result is that indentation is largely missing after having processed a rule without a trigger.

I've tried asking Google's "AI" for hints, because this is largely black magic to me - I have no idea how to actually debug what the formatter does and why. But, as usual, what I get is mostly not working or pure BS. Anything I've tried seems to have made it consistently worse.

It seems like indentation collapses after it has parsed one triggerless rule. Please note that the action indentation isn't handled by the formatter (I replace the whole "script part" post formatting, because otherwise it would completely reformat the code), so it is unaffected - triggers and conditions however are unindented after processing the triggerless rule:

rule "Datum testen_test" uid="daum-testen-1"
when
	Item Dummy_11 changed to ON
then

	    var LocalDate endofCentury = LocalDate.of(2000, 01, 01)
	    var LocalDate just = LocalDate.now()

	    var diff = Period.between(endofCentury, just)
	    logInfo("datediff", "diff between  Years: {}  Months: {} Days: {} - diff ist {}", diff.getYears, diff.getMonths, diff.getDays, diff)

	   val heute   = now.toLocalDate
end

rule "DSL Test" uid="DSL-test" [tag3, Tag2, Testing]
when
	Time is noon or
	Time is 17:38 or
	Item MySwitch received command or
	Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything")
end

rule "DSL Test copy" uid="56363d5e41" [tag3, Tag2, Testing]
when
	Time is noon or
	Time is 17:38 or
	Item MySwitch received command or
	Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything")
end

rule "Empty Test Rule" uid="5575458e42"
then
	return null;
end

rule "Initialize demo items" uid="demo-rule"
when
System started
then
	DemoLocation.postUpdate("52,9")
	DemoContact.postUpdate(OPEN)
	DemoString.postUpdate("Hello SmartHome!")
	DemoDateTime.postUpdate(new DateTimeType())
	DemoNumber.postUpdate(12.34)
end

rule "Managed DSL Test" uid="managedDSL" [tag3, Tag2, Testing]
when
Time is noon or
Time is 17:49 or
Item MySwitch received command or
Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything useful")
end

rule "mode TV" uid="mode-tv-1"
when
Item TvPower changed from OFF to ON
then
	logInfo("Rule","mode TV2")
	// STOP Sonos salon
	SonosSalonStop.sendCommand(ON)
end

rule "New Rule" uid="2335a6e4f9"
when
Item Dummy_11 changed to ON
then

end

rule "RunRule" uid="run-other"
then
	var shared = getRule("shared-test-1")
	logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
	shared.setEnabled(true)
	logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
	shared.run(false, "someVal", "Some value")
	logInfo("test", "ThingRegistry: " + getThingRegistry())
	logInfo("test", "MetadataRegistry: " + getMetadataRegistry())
end

rule "Self contained rule 1" uid="two-non-shared-1"
when
Item MySwitch received command
then
	logInfo("test", "Global variable is not")
end

rule "Self contained rule 2" uid="two-non-shared-2"
when
Item MySwitch received command
then
	logInfo("test", "Global variable is not in use")
end

rule "System started" uid="systemstart-1"
when
System reached start level 100
then
	logDebug('l', 'System reached start level 100')
end

rule "test" uid="multi-trigger-1"
when
Item test1 changed or
Item test2 changed or
Item test3 changed or
Item test4 changed or
Item test5 changed or
Item test6 changed
then
	switch(triggeringItemName) {
	    case "test1" : if(example1.state == ON) stuff.sendCommand(ON)
	    case "test2" : if(example2.state == ON) stuff.sendCommand(ON)
	    case "test3" : if(example3.state == ON) stuff.sendCommand(ON)
	    case "test4" : if(example4.state == ON) stuff.sendCommand(ON)
	    case "test5" : if(example5.state == ON) stuff.sendCommand(ON)
	    case "test6" : if(example6.state == ON) stuff.sendCommand(ON)
	}
end

rule "Test DSL if failure" uid="test-failure"
when
Item ControlSignal changed
but only if
Day is holiday and
Day is weekday
then
	val hour = java.time.ZonedDateTime.now().hour
	if (hour>=1 && hour<=7) {
	   logInfo("test", "Success")
	}
end

I don't know what to do about the formatting. I've tried everything I can think of, even trying to get input from both Google's "AI" and Copilot. They both suggest basically the same things, which don't work.

It seems like whenever a rule without a trigger is encountered, the "indentation tracker" errors and the following rules are without indentation. Copilot even suggested that this might be a bug in Xtext, and I've had the same thought. Individually, rules are formatted correctly, but when several are exported "together", the rules that follow a rule without a trigger are basically without indentation. This shouldn't "be possible" to achieve simply by doing something wrong in the formatter.

Indentations are all relative, I've found no way to set an absolute indentation. When there is a trigger, indentation is increased after "when" and then decreased before "then". Then it's increased again after "then" and decreased before "end". But, when there's no trigger, the first "rule" it will meet is the decrement before "then" - but it's already at indentation level 0 at that point. The bug might therefore be a result of the indentation "becoming negative". If we could set absolute indentation levels, we could set it to 0 before "then" and avoid the whole problem, but that doesn't seem to be an option.

We're running Xtext 2.43.0.M2, which isn't a proper release. I'm questioning if it's a good idea to run milestone/snapshot versions, but for all I know, the behavior is the same in 2.42.0. I don't know how to test that without building core in its entirety, so I haven't done so.

I'll push the updated code despite the formatting problems. I doubt that they are directly related to this PR, so I suggest that the PR is reviewed despite this problem.

Review in progress.
Remaining: FileFormatResource, DslRuleConverter, RulesFormatter, YamlConfigDescriptionParameterDTO, YamlRuleConverter, YamlRuleTemplateConverter, YamlRuleDTO, YamlRuleTemplateDTO, feature.xml and all YAML unit tests.

The Java25 build failure seems to be caused by flaky integration tests, the Java21 build passed, and I've changed nothing in the failing bundle in the latest commit.

It seems like I was able to fix the formatter with a little bit of "hacking". I converted the formatter from Xtext to Java, so that I have any idea of what I'm doing. I then overrode parts of the underlying logic so that I got access to where the indentation level go negative (it was indeed the problem). I then simply make it never go below 0, and the problem appears to be gone 🥳

rule "Datum testen_test" uid="daum-testen-1"
when
	Item Dummy_11 changed to ON
then

	    var LocalDate endofCentury = LocalDate.of(2000, 01, 01)
	    var LocalDate just = LocalDate.now()

	    var diff = Period.between(endofCentury, just)
	    logInfo("datediff", "diff between  Years: {}  Months: {} Days: {} - diff ist {}", diff.getYears, diff.getMonths, diff.getDays, diff)

	   val heute   = now.toLocalDate
end

rule "DSL Test" uid="DSL-test" [Tag2, Testing, tag3]
when
	Time is noon or
	Time is 17:38 or
	Item MySwitch received command or
	Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything")
end

rule "DSL Test copy" uid="56363d5e41" [Tag2, Testing, tag3]
when
	Time is noon or
	Time is 17:38 or
	Item MySwitch received command or
	Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything")
end

rule "Empty Test Rule" uid="5575458e42"
then
	return null;
end

rule "Initialize demo items" uid="demo-rule"
when
	System started
then
	DemoLocation.postUpdate("52,9")
	DemoContact.postUpdate(OPEN)
	DemoString.postUpdate("Hello SmartHome!")
	DemoDateTime.postUpdate(new DateTimeType())
	DemoNumber.postUpdate(12.34)
end

rule "Managed DSL Test" uid="managedDSL" [Tag2, Testing, tag3]
when
	Time is noon or
	Time is 17:49 or
	Item MySwitch received command or
	Time cron "0 0 10/2 * * ? *"
then
	logInfo("testing", "Not really doing anything useful")
end

rule "mode TV" uid="mode-tv-1"
when
	Item TvPower changed from OFF to ON
then
	logInfo("Rule","mode TV2")
	// STOP Sonos salon
	SonosSalonStop.sendCommand(ON)
end

rule "New Rule" uid="2335a6e4f9"
when
	Item Dummy_11 changed to ON
then

end

rule "RunRule" uid="run-other"
then
	var shared = getRule("shared-test-1")
	logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
	shared.setEnabled(true)
	logInfo("test", shared.getUID() + " enabled: " + shared.isEnabled())
	shared.run(false, "someVal", "Some value")
	logInfo("test", "ThingRegistry: " + getThingRegistry())
	logInfo("test", "MetadataRegistry: " + getMetadataRegistry())
end

rule "Self contained rule 1" uid="two-non-shared-1"
when
	Item MySwitch received command
then
	logInfo("test", "Global variable is not")
end

rule "Self contained rule 2" uid="two-non-shared-2"
when
	Item MySwitch received command
then
	logInfo("test", "Global variable is not in use")
end

rule "System started" uid="systemstart-1"
when
	System reached start level 100
then
	logDebug('l', 'System reached start level 100')
end

rule "test" uid="multi-trigger-1"
when
	Item test1 changed or
	Item test2 changed or
	Item test3 changed or
	Item test4 changed or
	Item test5 changed or
	Item test6 changed
then
	switch(triggeringItemName) {
	    case "test1" : if(example1.state == ON) stuff.sendCommand(ON)
	    case "test2" : if(example2.state == ON) stuff.sendCommand(ON)
	    case "test3" : if(example3.state == ON) stuff.sendCommand(ON)
	    case "test4" : if(example4.state == ON) stuff.sendCommand(ON)
	    case "test5" : if(example5.state == ON) stuff.sendCommand(ON)
	    case "test6" : if(example6.state == ON) stuff.sendCommand(ON)
	}
end

rule "Test DSL if failure" uid="test-failure"
when
	Item ControlSignal changed
but only if
	Day is holiday and
	Day is weekday
then
	val hour = java.time.ZonedDateTime.now().hour
	if (hour>=1 && hour<=7) {
	   logInfo("test", "Success")
	}
end

@lolodomo I force pushed just the last commit (unchanged) to trigger a rebuild because of the Java25 build failure. I hope that doesn't interfere with your review. It shouldn't, given that I pushed that commit a short while ago, and I doubt that you have started reviewing anything from that commit 🙏

I had to push one more change. This is the reason why I've waited with creating this PR, I figure out what changes must be done to the REST API while I develop the UI code. I hope that there won't be any further changes needed, the UI work is almost completed.

This change is "a bit dodgy". On the getRules endpoint, there's a summary parameter. If specified, the returned rules are stripped of all fields that's not in uid,templateUID,templateState,name,visibility,description,status,tags,configuration,editable. I just added configuration to the list. I assume that this is a measure to limit network traffic, the rules overview page use this setting. I do however need configuration to be able to figure out what YAML export options are valid, which is why I had to add it.

The "dodgy part" is that, this endpoint allows querying the "summary" even if the user isn't an admin. I suppose that there has been some kind of security consideration mixed in here:

        if ((summary == null || !summary) && !securityContext.isUserInRole(Role.ADMIN)) {
            // users may only access the summary
            return JSONResponse.createErrorResponse(Status.UNAUTHORIZED, "Authentication required");
        }

The "problem" now is that by adding configuration to the list, the configuration will also be available for non-admin users. Whether or not this matters I don't know, but it's an unfortunate mix of concerns.

If configuration isn't added to the summary, there are two other options to make this work: Have the overview page fetch all data, wasting bandwidth, or redesign the endpoint to have yet another option, so that it's possible to retrieve a reduced list of rules that is identical to "summary" except that it also includes configuration. This one could then require admin privileges, but it's a path I'd rather not go down, because it will just be "a mess" of parameters that is hard to make sense of for consumers.

So, is it OK to include configuration here or not? If not, what is the alternative solution?

I just realized, that in DSL Rules in trigger 'Time' 'is' time=TIME and condition Time is between start=TIME and end=TIME, TIME must be in the format hh:mm. That is, the hours must always be two digits. I do not know if this is considered here, when YAML rules are converted to DSL Rules. At the same time I could not find examples in the current, which demonstrate hours before 10:00, which are not noon ⇔ 00:00.

I just realized, that in DSL Rules in trigger 'Time' 'is' time=TIME and condition Time is between start=TIME and end=TIME, TIME must be in the format hh:mm. That is, the hours must always be two digits. I do not know if this is considered here, when YAML rules are converted to DSL Rules. At the same time I could not find examples in the current, which demonstrate hours before 10:00, which are not noon ⇔ 00:00.

This should be checked, however I don't have this branch checked out now, so I can't see how it behaves. Because of the Californium issue I can't really check this branch out without making a mess locally either, because this branch still uses the "M6 version" of Californium 4. But, I can't rebase the branch either, because @lolodomo has started to review.

I think I must wait for the review to finish, then rebase, and then verify how this is handled.

I just realized, that in DSL Rules in trigger 'Time' 'is' time=TIME and condition Time is between start=TIME and end=TIME, TIME must be in the format hh:mm. That is, the hours must always be two digits. I do not know if this is considered here, when YAML rules are converted to DSL Rules. At the same time I could not find examples in the current, which demonstrate hours before 10:00, which are not noon ⇔ 00:00.

I've copied this branch to another branch I've rebased so that I'm able to run it, and tested it. It didn't work, so I've made a fix:

That said, the UI doesn't really handle single digit hours very well either, so I have my doubt that the rest of the system does. It wouldn't surprise of such a trigger or condition won't actually work. But, that's not for this PR to deal with.

I'll commit the fix in the other branch and then try to cherry-pick it over to the PR branch so I can push it here.

the UI doesn't really handle single digit hours very well either

This does not matter. Two-digits hours are required only by the DSL Rules.xtext parser.

@lolodomo How is your review going? This PR needs a rebase to be adapted to other PRs that have been merged.

I plan to finish my review this week.

@Nadahar : can you please rebase ?

can you please rebase ?

It took some work to adjust to the changes, especially in FileFormatResource, but it's done now.

These random test failures are a bit annoying, and I've seen the "audio" failure quite a lot lately. I can't see any relevant changes to the bundle...

I looked a bit at the failing test, and I can't quite pin down what happens. The failure seems to be mostly on Java 25, but by looking at AudioServlet I can see that it's not "properly thread-safe" (it mixes synchronization with ConcurrentHashMap), so I assume that there is a threading issue that leads to the stream not always being closed. I know too little of what the code does to properly analyze it, but it is annoying to have builds fail because of this.

It seems like there is a path where doGet() can remove a stream while removeTimedOutStreams() is processing it, and doGet() doesn't close the stream. But, since I can't reproduce the test failure locally, it's hard to know the exact conditions that cause the failure.